There are 1000s of NS_WARN_IF uses like this: > if (NS_WARN_IF(NS_FAILED(rv) { > ... > } But there are also a couple of hundred uses where it's used outside a condition, like this: > NS_WARN_IF(NS_FAILED(rv)); The latter causes a problem. Coverity has a useful warning where it identifies missing checks for functions whose return values are usually checked. Unfortunately, we get many false positives because of NS_warn_if_impl(). There's already a suggestion of how to deal with this in the codebase. About a quarter of the just-generate-a-warning cases look like this: > Unused << NS_WARN_IF(NS_FAILED(rv)); People have already decided that this kind of use is exceptional enough that it warrants the |Unused <<| to make it clear that it's deliberate. So let's make that mandatory by adding MOZ_MUST_USE for NS_warn_if_impl(), which is the function that underlies NS_WARN_IF.

This makes the use of |Unused <<| mandatory for NS_WARN_IF uses outside of conditions, which increases consistency. It also avoids lots of false positives for Coverity's CHECKED_RETURN warning.

This version fixes some compile errors that a try push found.

I like the idea of this but I think we'd be better served converting stuff like: > Unused << NS_WARN_IF(*aOffset < 0); > Unused << NS_WARN_IF(!mLast); to |NS_WARN_IF_FALSE| (or maybe some new version). This way we'd avoid evaluating the code on non-debug builds (as-is the warning is debug only anyhow). Of course this means we'd have to negate those statements. > Unused << NS_WARN_IF(aRv.Failed()); > Unused << NS_WARN_IF(NS_FAILED(rv)); are candidates as well, adding something like |NS_WARN_IF_FAILED(rv)| might make sense. All that said, although it seems like this would be a good time to make the change, I'd be okay just filing a follow up.

This change avoids lots of false positives for Coverity's CHECKED_RETURN warning, caused by NS_WARN_IF's current use in both statement-style and expression-style. In the case where the code within the NS_WARN_IF has side-effects, I made the following change. > NS_WARN_IF(NS_FAILED(FunctionWithSideEffects())); > --> > Unused << NS_WARN_IF(NS_FAILED(FunctionWithSideEffects())); In the case where the code within the NS_WARN_IF lacks side-effects, I made the following change. > NS_WARN_IF(!condWithoutSideEffects); > --> > NS_WARNING_ASSERTION(condWithoutSideEffects, "msg"); This has two improvements. - The condition is not evaluated in non-debug builds. - The sense of the condition is inverted to the familiar "this condition should be true" sense used in assertions. A common variation on the side-effect-free case is the following. > nsresult rv = Fn(); > NS_WARN_IF_(NS_FAILED(rv)); > --> > DebugOnly<nsresult rv> = Fn(); > NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Fn failed");

Comment on attachment 8788118 [details] [diff] [review] Use MOZ_MUST_USE with NS_warn_if_impl() Review of attachment 8788118 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for taking the time to make all the NS_WARNING_ASSERTION changes, it's much clearer now. ::: dom/presentation/PresentationSessionInfo.h @@ +139,5 @@ > mReason = aReason; > > // Notify session state change. > if (mListener) { > + Unused << Any reason for the DebugOnly deviation here? ::: toolkit/components/terminator/nsTerminator.cpp @@ +343,5 @@ > } > > for (size_t i = 0; i < ArrayLength(sShutdownSteps); ++i) { > DebugOnly<nsresult> rv = os->AddObserver(this, sShutdownSteps[i].mTopic, false); > #if defined(DEBUG) Can remove the #if ::: xpcom/glue/nsDebug.h @@ +41,5 @@ > + * However, note that the argument to this macro is evaluated in all builds. If > + * you just want a warning assertion, it is better to use NS_WARNING_ASSERTION > + * (which evaluates the condition only in debug builds) like so: > + * > + * NS_WARNING_ASSERT(NS_SUCCEEDED(rv), "operation failed"); nit: NS_WARNING_ASSERTION

Pushed by nnethercote@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/177f41cecedb Use MOZ_MUST_USE with NS_warn_if_impl(). r=erahm.

jorgk: I don't think this will cause problems with comm-central -- I did a test build, and I don't see any NS_WARN_IF occurrences in comm-central that aren't within |if| conditions -- but I'll needinfo you here just in case, so you know where to look if something does go wrong.

Thanks for the heads-up, much appreciated!

The latest Coverity run just occurred, and it says: > 250 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan. Yay! Most of these fixes will be due to this change. (10--20 is a more typical number of fixes for one of these reports.) This means the signal-to-noise ratio of the CHECKED_RETURN check should be a lot higher now.

(In reply to Nicholas Nethercote [:njn] from comment #10) > The latest Coverity run just occurred, and it says: > > > 250 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan. > > Yay! Most of these fixes will be due to this change. (10--20 is a more > typical number of fixes for one of these reports.) This means the > signal-to-noise ratio of the CHECKED_RETURN check should be a lot higher now. I think most of those fixes were because of the triage for move assignment operator implementation - all of them false-positive, around 80 issues and 70 issues of variables null checked after dereferences in code generated by codegen.py

Nicholas this is a very very very good patch to silence the false-positive generated by this case. As a side note what I plan to do is to correlate our in code variables annotation, for example the skip of uninitialized member variables check, with an annotation from coverity. Because the bulk of our issues is from this kind of checkers. Uint ctor